Add ocean basin labeling and masking framework from ClimaOcean#140
Add ocean basin labeling and masking framework from ClimaOcean#140simone-silvestri wants to merge 15 commits intomainfrom
Conversation
Port the refactored labeling framework from ClimaOcean while preserving the existing bathymetry caching infrastructure. Adds Barrier type for basin separation, label_ocean_basins with periodic/tripolar support, OceanBasinMask with predefined seeds and convenience functions for Atlantic, Indian, Pacific, Southern, and Arctic oceans. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
simone-silvestri
requested review from
glwagner and
navidcy
and removed request for
glwagner
|
Can these tools also be used to identify lakes, or any enclosed body? If so "ocean basin" may not be the right word. |
|
thanks for the review, I ll go through the comments one by one. |
|
I think all the review comments have been addressed: Changes made:
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Sorry to be so annoying about |
| """ | ||
| Barrier{W, E, S, N} | ||
|
|
||
| A rectangular geographic region used to separate connected water regions during labeling. |
There was a problem hiding this comment.
bounding box literally represents a rectangular geographic region
| zb_data = zb_cpu.data[1:Nλ, :, 1] | ||
| zb_parent = zb_data.parent |
There was a problem hiding this comment.
this reads weird. what is the type of zb_data here? should we be using view(zb_cpu, 1:N, :, 1)? I thought that the getindex would allocate a new array.
either way, def better to use parent(zb_data) to be more future proof.
|
|
||
| # Concatenate a copy of the eastern half on the left and the western half on the right. | ||
| # This is an O(Nλ × Nφ) CPU allocation, acceptable for the serial labeling step. | ||
| zb_parent = vcat(zb_parent[half:Nλ, :], zb_parent, zb_parent[1:half, :]) |
There was a problem hiding this comment.
@giordano has showed cases where rebinding a name (eg reusing the name zb_parent) produces compiler problems, so you probably need to introduce a new name like concatenated_zb
| # If barriers are specified, apply them to a copy of the bathymetry | ||
| if !isnothing(barriers) | ||
| # Create a temporary field with the modified bathymetry | ||
| zb_modified = Field{Center, Center, Nothing}(cpu_grid) |
There was a problem hiding this comment.
could probably also use similar(zb) -- may be more robust to different bathymetry types
| cpu_grid = on_architecture(CPU(), grid) | ||
|
|
||
| TX = topology(cpu_grid, 1) | ||
| zb = cpu_grid.immersed_boundary.bottom_height |
There was a problem hiding this comment.
this is specific to GridFittedBottom and PartialCellBottom. perhaps it will work when we have "top" partial cells too and cut cells?
Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com>
glwagner
left a comment
glwagner
left a comment
There was a problem hiding this comment.
I know I'm being annoying, but it's probably nice to get this right...
- Can we use
BoundingBoxto represnet barriers? BasinMaskmay be wrong name, you may want "Basin" so the mask ismask = basin.mask- There's a lot of F64 baked in, it looks dirty to me but maybe has a purpose?
- Also lots of broadcasting and CPU assumption which has caused issues in the past. Do we need this to work on GPU ever? Perhaps not, but if we have like 40m bathymetry global, would it fail? I'm also ok to merge as is and work on clean up later, if it works.
- I think there is also an assumption about the type of immersed boundary grid --- the main thing I think we want to consider is supporting ice shelves / top partial cells, which hopefully will come soon.
|
I should have answered all the comments. I think this is ready to review again. |
The comments may have been addressed in the code but are unanswered in text. Can you please respond to my comments and say what was done to address them? |
2 participants
Summary
ClimaOcean has developed a basin labeling and masking framework that enables identifying and isolating individual ocean basins (Atlantic, Pacific, etc.) from bathymetry data. This is needed for basin-specific diagnostics, regional restoring, and inpainting. Meanwhile, NumericalEarth had independently added a bathymetry caching system to avoid recomputing regridded bathymetry on every call.
This PR reconciles the two codebases by porting ClimaOcean's labeling advances into NumericalEarth while preserving the caching infrastructure, preparing for CliMA/ClimaOcean.jl#757 which will have ClimaOcean depend on NumericalEarth's Bathymetry module for this functionality.
Changes
Barriertype for geographically separating connected ocean basins (e.g. closing Drake Passage to isolate the Atlantic)label_ocean_basinswith support for periodic and tripolar grid boundary conditionsOceanBasinMaskwith predefined barriers and seed points, plus convenience functions:atlantic_ocean_mask,indian_ocean_mask,pacific_ocean_mask,southern_ocean_mask,arctic_ocean_maskremove_minor_basins!to delegate tolabel_ocean_basinsinstead of doing its own internal labelingBathymetryRegridding, JLD2 cache)OceanBasinMaskcreationTest plan
Barrierconstructors (explicit, keyword, meridional with width)label_ocean_basinswithout barriers produces a single connected oceanlabel_ocean_basinswith barriers separates basins correctlyOceanBasinMaskfor the Atlantic excludes Pacific cells